Remove 4267 suppression from mono #66552
Conversation
am11
requested review from
BrzVlad,
SamMonoRT,
lambdageek,
lateralusX,
marek-safar,
naricc,
thaystg and
vargaz
as code owners
ghost
added
community-contribution
|
AOT failure is unrelated, opened #66556. |
|
@AaronRobinsonMSFT, thanks for your feedback. The general concern is that changing type on left side or function prototypes entails updating all function calls then callers of those functions. Also, sometimes it may not be possible to make such a change (without more code); e.g. on Windows, |
|
@AaronRobinsonMSFT, @lambdageek, the last update is following this pattern: int len = (int)strlen (foo);
emit_int32(&buf, foo);After size_t len = strlen (foo);
emit_int32(&buf, (int)foo);i.e. typecast on the line where it is needed. |
|
I think I have addressed the feedback and then some. The existing code is full of inconsistencies, so the feedback is bit subjective. I suggest to leave it to this state (unless, of course, there is a technical issue). |
|
One thing that came to mind when looking through the changes, since we are mitigating valid warnings using down cast and change of singed/unsigned types, maybe an option could be to cast through a macro or similar and then we could at least make some validation on debug builds that the cast is valid from type x to y? Just a thought since we are going over this. |
Jupp, understood, I just wrote down the differences that I spotted when looking through changes. If we do an explicit cast I think we should cast to the expected type as much as possible. |
| #pragma warning(disable:4090) // const problem | ||
| #pragma warning(disable:4146) // unary minus operator applied to unsigned type, result still unsigned | ||
| #pragma warning(disable:4244) // integer conversion, possible loss of data | ||
| #pragma warning(disable:4267) // integer conversion, possible loss of data |
There was a problem hiding this comment.
I wonder how painful it would be to enable -Wshorten-64-to-32 on clang - it would help catch some of these errors when building on OSX. Not a lot of the mono team works with the Win32 builds
There was a problem hiding this comment.
1674 hits on M1 (osx-arm64).. project for another weekend. 😅
There was a problem hiding this comment.
I guess -Wshorten-64-to-32 is more like C4244 at W3 (which we also need to fix). Looking at a sample compilation there's a couple hundredthousand warnings.
It looks like the root cause for a bunch of them is that mono_mempool_alloc and mono_mempool_alloc0 take an unsigned int size argument and all the macros that ultimately expand to that are passed size_t-shaped arguments in a lot of places.
Annoyingly mono_mempool_alloc is tagged MONO_API so there might be embedders calling it, so we can't just change its signature.
lateralusX
left a comment
lateralusX
left a comment
There was a problem hiding this comment.
LGTM, thanks for working through this!
|
Failure is #66556 |
This reverts commit c3dbdea. This caused `mono-aot-cross.exe` to crash on windows, breaking all the windows/AOT tests. Issue: dotnet#66718
dotnet#66552 did incorrect switch to size_t from int in a sections that expects variable to go negative.
#66552 did incorrect switch to size_t from int in a sections that expects variable to go negative.
|
It did fix it so no need to revert :) |
* Remove 4267 suppression from mono * Fix a mono warning on linux arm64 * Remove trailing whitespaces from changeset
…66721) dotnet#66552 did incorrect switch to size_t from int in a sections that expects variable to go negative.
ghost
locked as resolved and limited conversation to collaborators
6 participants
Contributes to #66154
cc @AaronRobinsonMSFT, @lambdageek